chore: Refactoring/domain handler update 2/n #7401
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: David Porter <[email protected]>
Signed-off-by: David Porter <[email protected]>
Signed-off-by: David Porter <[email protected]>
Signed-off-by: David Porter <[email protected]>
davidporter-id-au
changed the title
davidporter-id-au
changed the title
|
|
||
| s.mockMetadataMgr.On("GetDomain", mock.Anything, mock.Anything).Return(getDomainResp, nil) | ||
| s.mockMetadataMgr.On("UpdateDomain", mock.Anything, mock.Anything).Return(nil) | ||
| s.mockArchivalMetadata.On("GetHistoryConfig").Return(archiver.NewArchivalConfig("enabled", dynamicproperties.GetStringPropertyFn("disabled"), false, dynamicproperties.GetBoolPropertyFn(false), "disabled", "some random URI")) |
Member
Author
There was a problem hiding this comment.
these calls are not being made during the failover path, so this expectation being removed is expected. We don't need to look at the archival configuration if we're doing a failover.
| FailoverTimeoutInSeconds: common.Int32Ptr(1), | ||
| }, | ||
| err: errInvalidGracefulFailover, | ||
| err: errInvalidFailoverNoChangeDetected, |
Member
Author
There was a problem hiding this comment.
this is not only a graceful error condition, it refers to any change which fails validation or changes, so updating here
davidporter-id-au
requested review from
3vilhamster,
Shaddoll,
demirkayaender,
dkrotx,
jakobht,
neil-xie,
sankari165,
shijiesheng and
taylanisikdemir
as code owners
Signed-off-by: David Porter <[email protected]>
Shaddoll
reviewed
Oct 31, 2025
common/domain/handler.go
Outdated
| } | ||
|
|
||
| // Check the failover cool down time | ||
| if lastUpdatedTime.Add(d.config.FailoverCoolDown(intendedDomainState.Info.Name)).After(now) { |
common/domain/handler.go
Outdated
| var failoverType constants.FailoverType = constants.FailoverTypeGrace | ||
|
|
||
| // Force failover cleans graceful failover state | ||
| if updateRequest.FailoverTimeoutInSeconds == nil { |
Member
There was a problem hiding this comment.
there is a updateRequest.FailoverTimeoutInSeconds != nil above, can we move this block to the else branch?
Member
Author
There was a problem hiding this comment.
yeah, good call
Signed-off-by: David Porter <[email protected]>
Signed-off-by: David Porter <[email protected]>
| expectedErr error | ||
| }{ | ||
| { | ||
| name: "local domain with valid config", |
Member
Author
There was a problem hiding this comment.
this validation function isn't being used on local domains anymore
Signed-off-by: David Porter <[email protected]>
Signed-off-by: David Porter <[email protected]>
Shaddoll
approved these changes
Nov 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Second refactor of domain handler to move failover flow and Global domain update flow into separate flows and functions. It will be followed by another PR to remove the duplication with the failover endpoint
Why?
follow on from 1ec9f3b
How did you test it?
Potential risks
Release notes
Documentation Changes